Update Arrow to version 17#292
Conversation
| } else { | ||
| offsets = off_res.MoveValueUnsafe(); | ||
| } | ||
| if (!arrow::AllocateBuffer(arrow::default_memory_pool(), num_chars, &values).ok()) { |
There was a problem hiding this comment.
I've removed the arrow::default_memory_pool() here, assuming it will default to the default (although it doesn't explicitly state in the Arrow doc)
johanpel
left a comment
johanpel
left a comment
There was a problem hiding this comment.
Haven't been here for a while but LGTM overall.
If you are looking to revive this I think we should get CI working again first though, or is this necessary for that path forward?
There was a problem hiding this comment.
I see a lot of cosmetic changes here. It would be nice if you could add the associated clang tidy (I guess?) configuration you used to the project.
There was a problem hiding this comment.
Ok, I applied more clang-tidy suggestions and added the file that my editor used.
This also includes a change in some malloc APIs from size_t to int64_t, because the platform functions use int64_t there was a narrowing conversion needed. Now all of them use int64_t.
joosthooz
force-pushed
the
bump-arrow-17
branch
from
5a275b4 to
a5aee91
Compare
joosthooz
force-pushed
the
bump-arrow-17
branch
from
6478897 to
11662cd
Compare
joosthooz
force-pushed
the
bump-arrow-17
branch
from
4c50852 to
7ab8b9d
Compare
| FetchContent_Declare( | ||
| cerata | ||
| GIT_REPOSITORY https://github.com/abs-tudelft/cerata.git | ||
| GIT_TAG 0.0.11) |
There was a problem hiding this comment.
Had to change this tag to develop to make CI green, if we want we can create a new tag in cerata and then use that. It needs to be updated because a dependency of cerata used a very old cmake that is now incompatible.
There was a problem hiding this comment.
Added this very basic pyproject file to use the newer python -m build way of building instead of directly invoking setup.py. However it is very difficult to fully migrate to pyproject.toml because we need to build an external module, and the runtime needs to add the echo platform .so explicitly because it is not linked (dynamically loaded during runtime).
I've tried it but couldn't get it to work. So I've kept both codegen and runtime setup.py files.
|
|
||
| compile_units() | ||
|
|
||
| install(TARGETS fletcher fletcher_c fletcher_common |
There was a problem hiding this comment.
These new install cmds are added to add a CMake config file that projects look for when trying to use fletcher. I'm honestly not fully sure how these are supposed to look like, so I followed a minimal example and it seems to work.
|
|
||
| // MMIO: | ||
| ASSERT_TRUE(platform->WriteMMIO(0, 0).ok()); | ||
|
|
There was a problem hiding this comment.
This is also a noteworthy change, it overrides stdin to write some data, because the echo platform normally asks the user for input (which wouldn't work in CI, of course).
…ency. Also applied more clang-tidy suggestions and added config file.
|
Great work @joosthooz, thanks for making CI work again |
2 participants
Updated the Arrow dependency to use version 17.
Note: we should have a look at using the new Arrow C data interface, it didn't exist yet at the time all this was created.